Rebase #155
Rebase #155
Conversation
…C_DISABLE_VIA_PRIVACY (#120)
* RDKEMW-8929: Refactor base ipc class Reason for change: Test Procedure: Risks: Signed-off-by: Kelvin Lu <[email protected]> * Updating based on Copilot review comments --------- Signed-off-by: Kelvin Lu <[email protected]>
There was a problem hiding this comment.
Pull request overview
This pull request represents a major "rebase" effort that refactors the codebase to remove numerous compile-time flags and replace them with runtime configuration. The changes focus on voice control features, power management, and device management infrastructure.
Changes:
- Removes compile-time flags (CTRLM_LOCAL_MIC, CTRLM_LOCAL_MIC_TAP, BEEP_ON_KWD_ENABLED, NETWORKED_STANDBY_MODE_ENABLED, etc.) and replaces them with runtime checks
- Refactors IPC/IARM infrastructure to use a common base class with API revision management
- Updates logging to use "AUTOMATION" variants of logging macros throughout the codebase
- Adds controller ID range management for RF4CE and BLE networks
- Introduces callback mechanism for audio streaming initiation
- Updates factory test infrastructure to support dynamic audio analysis plugins
Reviewed changes
Copilot reviewed 65 out of 66 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/voice/ipc/ctrlm_voice_ipc_iarm_thunder.cpp | Refactors voice IPC to use runtime feature detection, adds API revision setting |
| src/voice/ctrlm_voice_obj.cpp | Replaces compile flags with runtime configuration for local mic and beep features |
| src/voice/ctrlm_voice_obj.h | Adds new member variables for runtime feature flags and audio start callback support |
| src/rf4ce/ctrlm_rf4ce_network.cpp | Major refactoring to extract audio streaming logic, add controller ID range management |
| src/ble/ctrlm_ble_network.cpp | Similar refactoring for BLE audio streaming, adds controller ID migration logic |
| src/ipc/ctrlm_rcp_ipc_iarm_thunder.cpp | Updates to use new broadcast_iarm_event signatures and API revision management |
| src/ipc/ctrlm_ipc_iarm.h/cpp | Adds API revision management and refactors event broadcasting |
| src/factory/.c/.cpp | Updates factory test infrastructure for dynamic plugin loading |
| CMakeLists.txt | Removes numerous build options and simplifies configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if(it->response != CTRLM_THREAD_MONITOR_RESPONSE_ALIVE) { | ||
| XLOGD_TELEMETRY("Thread %s is unresponsive", it->name); | ||
| XLOGD_AUTOMATION_TELEMETRY("Thread %s is unresponsive", it->name); |
There was a problem hiding this comment.
The logging macro XLOGD_AUTOMATION_TELEMETRY is being used but may not be defined. This could cause compilation errors. Verify that this logging macro is properly defined in the logging infrastructure.
| if (it.second == CTRLM_IARM_CALL_RESULT_SUCCESS) { | ||
| return true; | ||
| } | ||
| } | ||
| return true; | ||
| return false; |
There was a problem hiding this comment.
The logic change for result checking appears to be inverted. The original code returned true if all results were SUCCESS and false if any failed. The new code returns true if ANY result is SUCCESS and false if all failed. This could cause incorrect behavior when processing multiple network results.
| false, NULL, NULL, NULL, (fd >= 0) ? true : false, true, NULL, NULL, | ||
| str_transcription.empty() ? NULL : str_transcription.c_str(), str_audio_file.empty() ? NULL : str_audio_file.c_str(), &request_uuid, request_config.low_latency, request_config.low_cpu_util, fd); |
There was a problem hiding this comment.
The function ctrlm_voice_ipc_iarm_thunder_t::voice_session_request has a complex function call signature with many parameters. Lines 770-771 add two NULL parameters before the transcription and audio file parameters. This changes the function signature but it's not clear if all call sites have been updated correctly. Verify that the function parameter order matches the declaration in the header file.
No description provided.